Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove duplicated breadcrumbs when syncing with iOS/macOS #1283

Merged
merged 16 commits into from
Feb 28, 2023
Merged

Remove duplicated breadcrumbs when syncing with iOS/macOS #1283

merged 16 commits into from
Feb 28, 2023

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Feb 13, 2023 •
edited
Loading

📜 Description

By fixing the issue that scope sync was never enabled on iOS with #1196, we introduced the issue that breadcrumbs could be duplicated because of bi-directional sync by calling addBreadcrumbs, and loading the native context with loadContexts to enrich events, which also includes the native breadcrumbs.

  • Create breadcrumbs with correct timestamp when syncing down to native layer through addBreadcrumbs
  • Remove duplicate breadcrumbs when syncing up from native layer through loadContexts

💡 Motivation and Context

Closes #1270

💚 How did you test it?

Added unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

This approach in this PR fixes the issue, but we now have to compare all breadcrumbs with each other, which could get slow with a larger number of breadcrumbs.

Another way would be to just remove syncing breadcrumbs down to the native layer.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 13, 2023 •
edited
Loading

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 285.38 ms 344.74 ms 59.36 ms
Size 6.06 MiB 7.09 MiB 1.03 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8fa3934 340.64 ms 407.92 ms 67.28 ms
fbf42af 323.04 ms 365.09 ms 42.04 ms
21d4150 379.31 ms 449.23 ms 69.93 ms
08a7b4f 346.47 ms 403.29 ms 56.82 ms
9811573 309.76 ms 402.60 ms 92.84 ms
c70e01a 331.04 ms 401.46 ms 70.42 ms
48e79fd 354.22 ms 391.46 ms 37.24 ms
3637a22 322.59 ms 390.00 ms 67.41 ms
4efee31 308.92 ms 368.68 ms 59.76 ms
6d7a391 331.94 ms 367.04 ms 35.10 ms

App size

Revision Plain With Sentry Diff
8fa3934 6.06 MiB 7.09 MiB 1.03 MiB
fbf42af 5.94 MiB 6.96 MiB 1.02 MiB
21d4150 5.94 MiB 6.97 MiB 1.03 MiB
08a7b4f 5.94 MiB 6.95 MiB 1.01 MiB
9811573 5.94 MiB 6.96 MiB 1.02 MiB
c70e01a 5.94 MiB 6.97 MiB 1.03 MiB
48e79fd 5.94 MiB 6.95 MiB 1.01 MiB
3637a22 6.06 MiB 7.09 MiB 1.03 MiB
4efee31 5.94 MiB 6.92 MiB 1003.76 KiB
6d7a391 5.94 MiB 6.95 MiB 1.01 MiB

Previous results on branch: fix/duplicate-breadcrumbs-logging

Startup times

Revision Plain With Sentry Diff
40f188e 330.40 ms 361.53 ms 31.14 ms
130de29 301.22 ms 354.31 ms 53.09 ms
8d570b0 310.31 ms 368.27 ms 57.96 ms
91e64ae 316.44 ms 359.84 ms 43.40 ms
c910142 317.40 ms 355.68 ms 38.28 ms
a4d5fe6 302.13 ms 378.06 ms 75.94 ms
a638fd5 339.84 ms 379.14 ms 39.30 ms

App size

Revision Plain With Sentry Diff
40f188e 6.06 MiB 7.09 MiB 1.03 MiB
130de29 6.06 MiB 7.09 MiB 1.03 MiB
8d570b0 6.06 MiB 7.09 MiB 1.03 MiB
91e64ae 6.06 MiB 7.09 MiB 1.03 MiB
c910142 6.06 MiB 7.09 MiB 1.03 MiB
a4d5fe6 6.06 MiB 7.09 MiB 1.03 MiB
a638fd5 6.06 MiB 7.09 MiB 1.03 MiB

@github-actions
Copy link
Contributor

github-actions bot commented Feb 13, 2023 •
edited
Loading

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1252.86 ms 1267.53 ms 14.67 ms
Size 8.10 MiB 9.08 MiB 1004.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
21845e2 1279.37 ms 1298.81 ms 19.45 ms
b47809a 1252.61 ms 1278.57 ms 25.96 ms
49a149b 1296.47 ms 1320.20 ms 23.73 ms
5603ab2 1268.47 ms 1280.73 ms 12.26 ms
b728df4 1287.43 ms 1293.94 ms 6.51 ms
21d4150 1252.86 ms 1280.55 ms 27.69 ms
1edf30e 1254.43 ms 1272.82 ms 18.39 ms
ccc09e4 1254.74 ms 1277.08 ms 22.34 ms
a49594a 1284.83 ms 1313.29 ms 28.45 ms
c70e01a 1273.00 ms 1299.12 ms 26.12 ms

App size

Revision Plain With Sentry Diff
21845e2 8.15 MiB 9.12 MiB 991.34 KiB
b47809a 8.16 MiB 9.17 MiB 1.01 MiB
49a149b 8.15 MiB 9.12 MiB 986.26 KiB
5603ab2 8.15 MiB 9.12 MiB 990.57 KiB
b728df4 8.15 MiB 9.15 MiB 1020.72 KiB
21d4150 8.16 MiB 9.17 MiB 1.01 MiB
1edf30e 8.16 MiB 9.17 MiB 1.01 MiB
ccc09e4 8.16 MiB 9.16 MiB 1.01 MiB
a49594a 8.16 MiB 9.16 MiB 1.00 MiB
c70e01a 8.16 MiB 9.17 MiB 1.01 MiB

Previous results on branch: fix/duplicate-breadcrumbs-logging

Startup times

Revision Plain With Sentry Diff
40f188e 1270.11 ms 1275.53 ms 5.42 ms
8d570b0 1260.57 ms 1286.88 ms 26.31 ms
c910142 1232.19 ms 1265.57 ms 33.39 ms
a4d5fe6 1267.67 ms 1285.23 ms 17.57 ms
130de29 1251.83 ms 1281.11 ms 29.28 ms
91e64ae 1242.90 ms 1254.87 ms 11.98 ms
a638fd5 1247.36 ms 1263.37 ms 16.01 ms

App size

Revision Plain With Sentry Diff
40f188e 8.10 MiB 9.08 MiB 1004.40 KiB
8d570b0 8.10 MiB 9.08 MiB 1003.32 KiB
c910142 8.10 MiB 9.08 MiB 1004.28 KiB
a4d5fe6 8.10 MiB 9.08 MiB 1004.50 KiB
130de29 8.10 MiB 9.08 MiB 1003.55 KiB
91e64ae 8.10 MiB 9.08 MiB 1003.30 KiB
a638fd5 8.10 MiB 9.08 MiB 1004.35 KiB

@codecov
Copy link

codecov bot commented Feb 14, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.96 ⚠️

Comparison is base (2d3b03d) 89.83% compared to head (340d779) 88.87%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1283      +/-   ##
==========================================
- Coverage   89.83%   88.87%   -0.96%     
==========================================
  Files         155      121      -34     
  Lines        5064     3810    -1254     
==========================================
- Hits         4549     3386    -1163     
+ Misses        515      424      -91
Impacted Files Coverage Δ
...ib/src/integrations/load_contexts_integration.dart
.../src/user_interaction/user_interaction_widget.dart
...ter/lib/src/integrations/on_error_integration.dart
flutter/lib/src/sentry_flutter_options.dart
.../src/integrations/widgets_binding_integration.dart
flutter/lib/src/renderer/renderer.dart
...r/lib/src/integrations/screenshot_integration.dart
flutter/lib/src/sentry_native_channel.dart
flutter/lib/src/native_scope_observer.dart
...r/lib/src/integrations/native_sdk_integration.dart
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@denrase denrase marked this pull request as ready for review February 14, 2023 09:11
brustolin
brustolin approved these changes Feb 16, 2023
View reviewed changes
Copy link
Collaborator

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

marandaneto
marandaneto approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @denrase

marandaneto
marandaneto reviewed Feb 28, 2023
View reviewed changes
flutter/test/integrations/init_native_sdk_integration_test.dart Outdated
@@ -53,7 +53,7 @@ void main() {
'maxCacheItems': 30,
'sendDefaultPii': false,
'enableOutOfMemoryTracking': true,
'enableNdkScopeSync': false,
'enableNdkScopeSync': true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denrase is this intentional?

@marandaneto marandaneto merged commit d2f62d6 into main Feb 28, 2023
@marandaneto marandaneto deleted the fix/duplicate-breadcrumbs-logging branch February 28, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marandaneto marandaneto marandaneto approved these changes

@brustolin brustolin brustolin approved these changes

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicated BreadCrumbs from sentry_logging
3 participants
@denrase @marandaneto @brustolin